Route kiln test through docker-virtual registry and refactor test-script generation by rizwanreza · Pull Request #643 · pivotal-cf/kiln

rizwanreza · 2026-04-30T20:18:20Z

Summary

Route kiln test image pulls through the internal Broadcom Artifactory docker-virtual registry, eliminating the need for a separate docker login before running tile tests.
Rebuild the test-script generation layer (testPlan / suiteStep) so each suite runs in its own subshell with an independent exit code, colored pass/fail summary, and optional per-suite timestamps.
Add --verbose / --silent / --stability flags; quiet npm by default; set GOMAXPROCS inside the container.
Refactor for maintainability: decompose runTest, fix naming inconsistencies, and clean up small code smells.

What changed

Dockerfile (`internal/test/Dockerfile`)

All FROM lines now use tas-rel-eng-docker-virtual.usw1.packages.broadcom.com (the same registry as AuthConfigs) so kiln test can pull base images without a prior docker login.
Stable tool layers (Go runtime, jq, nodejs, npm) are ordered before the ARG ARTIFACTORY_USERNAME declaration so they survive credential rotation.
ginkgo@v1.16.5 is pinned; GOPROXY credentials are scoped to a single RUN step (not exported as ENV) to keep that layer stable.
ops-manifest gem install credentials are exported to ENV for runtime use.

`kiln test` behaviour

New flags: --stability (run only stability tests), --silent (suppress kiln info lines), --verbose (print container ID and per-suite timestamps).
--verbose defaults to false; npm runs --silent / ci --silent by default and uses the louder form only when --verbose is passed.
GOMAXPROCS is set to the host's CPU count inside the container.
Credentials may be supplied via -e ARTIFACTORY_USERNAME=... -e ARTIFACTORY_PASSWORD=... or exported in the shell; kiln test exits with an error before contacting Docker if either is missing.

`internal/test/container.go` refactors

RunMetadata → RunStability: field name now matches the --stability flag and "Stability Tests" suite label.
Decompose runTest: extracted buildTestImage (image build + registry auth) and startAndWaitContainer (container lifecycle + log drain + signal handling) so runTest is a 30-line orchestrator.
Single credential resolution: requiredArtifactoryCredentialsFromMap reuses the already-decoded environmentVars map, eliminating a redundant decodeEnvironment call.
verbose as a render parameter: removed verbose from the testPlan struct; testPlan is now pure data and script(verbose bool) makes the rendering concern explicit.
GinkgoFlags typo fixed: TileTest.Options.GingkoFlags → GinkgoFlags.
errors.New: replaced fmt.Errorf("%s", detail) in checkImageBuildResponse.
filepath consistency: removed the "path" import; path.Dir/Base replaced with filepath.Dir/Base for host filesystem operations.

README

Updated kiln test docs to reflect the new flags (--stability, --ginkgo-flags, --manifest, --migrations), the credential flow through docker-virtual, and the DockerVirtualRegistryHost alignment requirement.

`kiln test` output excerpt

`--verbose` flag

SUCCESS! -- 520 Passed | 0 Failed | 0 Pending | 4 Skipped


Ginkgo ran 1 suite in 8m46.345003494s
Test Suite Passed
[21:13:42] Completed: Manifest Tests

[21:03:23] ✓ Migration Tests Passed
[21:04:55] ✓ Stability Tests Passed
[21:13:42] ✓ Manifest Tests Passed

without `--verbose` flag

SUCCESS! -- 520 Passed | 0 Failed | 0 Pending | 4 Skipped


Ginkgo ran 1 suite in 8m10.665858116s
Test Suite Passed

✓ Migration Tests Passed
✓ Stability Tests Passed
✓ Manifest Tests Passed

notrepo05 · 2026-05-01T00:01:55Z


-	logger.Println("reading image build response")
-	if err := checkImageBuildResponse(resp.Body); err != nil {
+	if err := checkImageBuildResponse(resp.Body, nil); err != nil {


nit: Can we refactor checkImageBuildResponse to take one argument again? Or maybe remove the image build log section in the README.md for now

notrepo05 · 2026-05-01T00:04:00Z

+		envMap["ARTIFACTORY_PASSWORD"] = configuration.ArtifactoryPassword
+	}
+
 	artifactoryUsername := envMap["ARTIFACTORY_USERNAME"]


Nitpick: If the proxy is ever not needed I'd recommend also supporting reading the ARTIFACTORY_USERNAME/PASSWORD via the OS environment too. If it isn't passed then these secrets are only configured from -e

We've assumed the proxy is always needed, but the env vars are also read from the shell.

notrepo05 · 2026-05-01T00:13:47Z

Works well and has a simple user experience. Thank you!

Remove --broadcom-proxy. The embedded Dockerfile pins FROM lines on\ntas-rel-eng-docker-virtual and sets GOPROXY/GOSUMDB from ARTIFACTORY\nbuild args. Kiln always requires ARTIFACTORY_USERNAME and\nARTIFACTORY_PASSWORD, passes them as the only ImageBuild build args,\nand sends AuthConfigs for DockerVirtualRegistryHost so pulls work\nwithout docker login. Retains quiet image build output and the\nContainerWait(next-exit) fix for AutoRemove. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

Restructure Dockerfile so stable tool layers (jq, node, npm) precede credential-dependent steps; scopes GOPROXY to the ginkgo RUN command to avoid cache-busting on credential rotation; pins ginkgo to v1.16.5 for reproducibility; sets CGO_ENABLED=0 to avoid gcc issues on arm64. Run each test suite (migration, stability, manifest) as a separate shell invocation so output is never interleaved. Add printed suite headers for migration and stability suites that do not emit their own Ginkgo header. Prefer npm ci when package-lock.json exists, and suppress audit/fund noise with --no-audit --no-fund otherwise. Expose GOMAXPROCS in the container to utilise all available cores. Remove log.Logger in favour of plain fmt.Fprintln progress lines, and show the container ID only when --verbose is set. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

…mmary Replace the flat command slice with a testPlan struct that holds a fail-fast setup block and an ordered list of named suiteSteps. Each suite runs in its own bash subshell so a cd in one suite cannot affect another and exit codes are captured individually. When more than one suite is selected, script() appends a pass/fail summary with ANSI green ✓ / red ✗ markers and a completion timestamp for each suite. When only one suite runs the summary is omitted. The script always exits non-zero if any individual suite failed. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

Default --verbose to false so kiln test output stays clean. When verbose is off, npm ci/install runs with --silent to suppress progress bars and deprecation warnings. When verbose is on, npm output is fully visible and the script emits a timestamped Starting/Completed line around each suite so it is easy to see how long each phase takes. The verbose flag is propagated into testPlan so script() can toggle these behaviours without threading configuration deeper into helpers. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

The field was set by --stability, labels the suite "Stability Tests", and is described as stability everywhere. The old name conflicted with all of that, making the code harder to read. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

RequiredArtifactoryCredentials was re-parsing configuration.Environment after runTest had already decoded it into envMap. Extract an unexported requiredArtifactoryCredentialsFromMap that works on the decoded map, and call it from runTest. Drop the redundant artifactoryUsername/Password alias variables; buildArgs now holds pointers to the resolved strings directly. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

runTest was a 145-line function covering five concerns: daemon ping, environment parsing, image build, container lifecycle, and log streaming. Extract buildTestImage (image build and registry auth) and startAndWaitContainer (create, start, signal handling, log drain, wait) so each unit is independently readable and testable. runTest becomes a 30-line orchestrator. Also removes the now-unused "path" import; path.Dir/Base were replaced with filepath.Dir/Base during extraction. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

The struct field TileTest.Options.GingkoFlags was silently renamed when assigned to Configuration.GinkgoFlags. Correcting the typo removes the inconsistency. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

testPlan is a pure data value describing which suites to run. Verbosity is a rendering concern: it only affects the generated shell script, not what tests are selected. Changing the signature to script(verbose bool) makes this explicit at the call site and removes the hidden coupling between plan construction and script rendering. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

…Response fmt.Errorf with a plain %s verb (no wrapping) triggers go vet's printf-style warning and adds unnecessary overhead. errors.New is the correct call when there is no cause to wrap. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

fmt.Fprintln/Fprintf writes to the status writer are informational only; failing the operation because a progress line could not be printed would be incorrect. Use _, _ = to make the intentional discard explicit and satisfy errcheck. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

_timeN was always captured and always interpolated into the [HH:MM:SS] prefix of the pass/fail summary lines, regardless of the verbose flag. Timestamps are a verbose-only concern: gate _timeN capture on verbose and produce two summary formats — plain (✓/✗ Suite Passed/Failed) when not verbose, timestamped when verbose. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

The previous description only mentioned the container ID. Verbose also controls per-suite timestamps, the timestamped summary footer, and npm install verbosity. The new text lists all four effects. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

notrepo05

This works as expected running against real tiles. The fast fail is a nice ux improvement and the summary is easier to read!

I noticed that the integration test, due to network conditions, is failing. I would suggest making the verbosity more configurable so the errors related to that are visible:

diff --git a/internal/test/integration_test.go b/internal/test/integration_test.go
index c7bbb4f3..f410461b 100644
--- a/internal/test/integration_test.go
+++ b/internal/test/integration_test.go
@@ -50,6 +50,7 @@ func TestDockerIntegration(t *testing.T) {
 			AbsoluteTileDirectory: tmpDir,
 			RunAll:                true,
 			Environment:           []string{"ARTIFACTORY_USERNAME=" + artifactoryUsername, "ARTIFACTORY_PASSWORD=" + artifactoryPassword},
+			Verbose:               testing.Verbose(),
 		}
 		out := io.Discard
 		if testing.Verbose() {

But after applying the review comments below the test suite passes.

I also left a nit on a test setup.

Thank you!

notrepo05 · 2026-05-06T01:17:20Z

+			DeferCleanup(func() {
+				if hasU {
+					Expect(os.Setenv("ARTIFACTORY_USERNAME", savedU)).To(Succeed())
+				} else {
+					Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed())
+				}
+				if hasP {
+					Expect(os.Setenv("ARTIFACTORY_PASSWORD", savedP)).To(Succeed())
+				} else {
+					Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed())
+				}
+			})
+			Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed())
+			Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed())


Does the BeforeEach() set these environment variables before every test is executed? If so, we might be able to get away with

Suggested change

DeferCleanup(func() {

if hasU {

Expect(os.Setenv("ARTIFACTORY_USERNAME", savedU)).To(Succeed())

} else {

Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed())

}

if hasP {

Expect(os.Setenv("ARTIFACTORY_PASSWORD", savedP)).To(Succeed())

} else {

Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed())

}

})

Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed())

Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed())

t := GinkgoT()

t.Setenv("ARTIFACTORY_USERNAME", "")

t.Setenv("ARTIFACTORY_PASSWORD", "")

Fixed. I like this!!

Co-authored-by: Nick Rohn <nick.rohn@broadcom.com>

Replace manual os.LookupEnv/DeferCleanup save-restore pattern with GinkgoT().Setenv, which handles teardown automatically. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

The logOutput parameter was always passed nil at the only call site, and SuppressOutput: true on the ImageBuild call means the daemon never sends stream messages anyway. Drop the parameter and remove the dead write path. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

Set Verbose: testing.Verbose() on the integration test Configuration so build and container output is shown when running with -v, making network errors visible. Also tighten setupTestRepo: close the tar archive immediately after extraction (rather than in a deferred cleanup) and drop the redundant os.RemoveAll since t.TempDir() already manages the directory lifecycle. Switch assert to require for the setup calls so test failures are fatal. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

Resolves the conflict between the Broadcom proxy migration on this branch and the Ruby 3.4.8 → 4.0.3 bump on main. The Dockerfile now pulls ruby:4.0.3 via the internal docker-virtual registry. Updates the Dockerfile structure test to assert 4.0.3. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

SuppressOutput was hardcoded to true and the logOutput writer was dropped when buildTestImage was extracted, silencing Docker build progress even with --verbose. Thread verbose through buildTestImage and restore the stream writer so build steps are visible when the flag is set. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

notrepo05 reviewed May 1, 2026

View reviewed changes

rizwanreza force-pushed the add-broadcom-docker-proxy-to-kiln-test branch from b47ee0b to b910390 Compare May 1, 2026 15:36

rizwanreza and others added 9 commits May 5, 2026 12:15

Fix GingkoFlags typo: rename to GinkgoFlags

b80e9da

The struct field TileTest.Options.GingkoFlags was silently renamed when assigned to Configuration.GinkgoFlags. Correcting the typo removes the inconsistency. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

rizwanreza changed the title ~~Broadcom proxy for kiln test~~ Route kiln test through docker-virtual registry and refactor test-script generation May 5, 2026

rizwanreza requested a review from notrepo05 May 5, 2026 17:44

rizwanreza and others added 3 commits May 5, 2026 12:46

rizwanreza requested a review from davewalter May 5, 2026 19:39

notrepo05 requested changes May 6, 2026

View reviewed changes

rizwanreza and others added 4 commits May 6, 2026 09:30

Add NPM virtual proxy

97b16f1

Co-authored-by: Nick Rohn <nick.rohn@broadcom.com>

Use t.Setenv for Artifactory credential cleanup in test

fe19ecb

Replace manual os.LookupEnv/DeferCleanup save-restore pattern with GinkgoT().Setenv, which handles teardown automatically. ai-assisted=yes Co-authored-by: Cursor <cursoragent@cursor.com>

rizwanreza requested a review from notrepo05 May 6, 2026 14:57

notrepo05 approved these changes May 6, 2026

View reviewed changes

rizwanreza and others added 2 commits May 7, 2026 14:38

rizwanreza merged commit 13c5e0f into main May 7, 2026
5 checks passed

rizwanreza deleted the add-broadcom-docker-proxy-to-kiln-test branch May 7, 2026 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Route kiln test through docker-virtual registry and refactor test-script generation - #643

Route kiln test through docker-virtual registry and refactor test-script generation#643
rizwanreza merged 19 commits into
mainfrom
add-broadcom-docker-proxy-to-kiln-test

rizwanreza commented Apr 30, 2026 •

edited

Loading

Uh oh!

notrepo05 May 1, 2026

Uh oh!

notrepo05 May 1, 2026 •

edited

Loading

Uh oh!

rizwanreza May 6, 2026

Uh oh!

notrepo05 commented May 1, 2026

Uh oh!

notrepo05 left a comment •

edited

Loading

Uh oh!

notrepo05 May 6, 2026

Uh oh!

rizwanreza May 6, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rizwanreza commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Dockerfile (internal/test/Dockerfile)

kiln test behaviour

internal/test/container.go refactors

README

kiln test output excerpt

--verbose flag

without --verbose flag

Uh oh!

notrepo05 May 1, 2026

Choose a reason for hiding this comment

Uh oh!

notrepo05 May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rizwanreza May 6, 2026

Choose a reason for hiding this comment

Uh oh!

notrepo05 commented May 1, 2026

Uh oh!

notrepo05 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notrepo05 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

rizwanreza May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rizwanreza commented Apr 30, 2026 •

edited

Loading

Dockerfile (`internal/test/Dockerfile`)

`kiln test` behaviour

`internal/test/container.go` refactors

`kiln test` output excerpt

`--verbose` flag

without `--verbose` flag

notrepo05 May 1, 2026 •

edited

Loading

notrepo05 left a comment •

edited

Loading

rizwanreza May 6, 2026 •

edited

Loading